Fix PMS atomic persistence and truthful save responses - #5666

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

1. Issue fit

Yes. #5664 asks for five things and the diff delivers all five: atomic whole-document replacement (writeJsonAtomically, QSaveFile with setDirectWriteFallback(false) — the right call, since a fallback direct write is exactly the truncation the issue is about); surfaced open/write/commit failures (reportPersistenceFailure → qCWarning(lcAx25) + activity); acknowledgement only after commit (cmdRead, cmdKill, finishCompose all build a candidate vector, save it, and adopt it only on success); m_nextId consumed only on a committed write; and the prior document preserved on failure.

I checked the "keep in-memory consistent with the last successful write" requirement specifically: m_messages is now assigned only via std::move(updated) after a true return in all three mutators — there is no path left that mutates m_messages and then saves. m_heard and m_callers are still mutated before their save and keep the mutation on failure, but the issue scopes that requirement to message data / read flags / next ID, and those two stores are self-healing (the next successful save carries the pending entry), so I do not read that as unaddressed.

Socket disclosure: the added test binds nothing. It is pure file I/O plus the existing in-memory onAirFrame/Frame::encode() injection seam — no QTcpServer/QUdpSocket/QLocalServer, no peer process, no Fake* radio. Nothing to record beyond that.

2. Scope

No CHANGELOG.md edit, no new settings key, no new RadioCapabilities bool, no build-config churn. The only thing in the diff no requirement strictly explains is the brace addition around m_callers.remove(...) in recordCaller (PmsMailbox.cpp:410-412) — one line of style, not worth a row.

3. Blockers

None.

4. Nits (non-blocking)

• A caller can be trapped in compose mode by a persistent storage failure — inline at PmsMailbox.cpp:771. Retaining the draft is the right trade over the old false SAVED, but there is no abort verb: finishCompose(false) has no caller anywhere in the file (so "Message aborted." is dead on main too), and onLinkData calls touchSession() on every byte, so the 10-minute idle timer re-arms with each retry. Bounded only by the caller sending DISC. No data is lost either way.
• The destructor's save can now emit a signal — inline at PmsMailbox.cpp:103. saveHeard was const and silent before this PR; it can now reach emit activity(...), and m_pms is a child of Ax25HfPacketDecodeDialog (Ax25HfPacketDecodeDialog.cpp:852, connected to appendSystemLine at :1253), so on the failure path the slot runs after the parent's derived destructor has completed.
• m_draft = candidate; at PmsMailbox.cpp:776 is a dead store — m_draft is fully reset by cmdSendBegin before it is next read. Harmless; it just reads as if it mattered.
• Sibling left behind, out of scope: src/core/tnc/HeardList.cpp:196 has the identical defect class — WriteOnly | Truncate with the f.write(...) result discarded. It is the TNC's own heard list, not one of the three PMS stores #5664 names, so this is a completeness note rather than something to bundle here.

5. What I tried to break

• "Atomic replacement" — I checked the fixture actually exercises the path it claims. The test makes the target path a directory. Qt's QSaveFile::open() has an explicit existingFile.isDir() → WriteError branch before it creates its temp file, so the failure is real and not an accident of permissions. And the hard-link assertions would fail against the unfixed QFile truncate (the link would observe the new bytes), so they are load-bearing rather than self-confirming.
• I tried to find a surviving path that mutates state before the save. Grepped every m_messages write and saveMessages call site in the head checkout: cmdRead, cmdKill, finishCompose and loadAll are the only ones, and the first three all adopt via std::move after a true. updated.remove(i) / updated[i] index a copy of m_messages taken in the same scope, so the indices cannot skew.
• I tried to break the ID accounting. finishCompose writes m_nextId + 1 while assigning candidate.id = m_nextId, and bumps the member only after commit; the test's nextId == 3 assertion after two commits and one failure would catch a consumed ID. loadAll additionally re-derives m_nextId = max(nextId, id+1), so even a stale header self-corrects.
• Multi-instance interference in the new test. The callerWriter / callerFailure scopes construct second PmsMailbox objects against the same AETHER_PMS_DIR while pms is still connected; I traced their destructors (saveHeard only) against the order of the durableHeard / durableCallers captures and found no fixture that reads a file another instance rewrites between capture and assertion.
• Could not check, and saying so: pms_mailbox_test appears in no ctest -R filter in .github/workflows/ci.yml, so the five green checks prove the new test compiles and links on Linux/macOS/Windows — not that it passes. In particular the std::filesystem::create_hard_link fixture is unexercised by CI and will hard-fail (not skip) on a filesystem without hard-link support. Worth one local Windows run before merge.
• All findings above are reasoned from the head checkout at /tmp/aetherclaude/pr-5666; I ran no build and executed no test.

6. Recommendation

Approve with nits. The fix matches the issue requirement-for-requirement, the seam it chose (candidate-vector → save → adopt) is the right one, it adds no public surface, and the test is adversarial enough to fail against the unfixed code. The compose-trap and destructor-emit items are worth a follow-up commit but neither breaks a user or violates canon. Next step: confirm the new test passes on Windows locally, then merge.

Nice work — the setDirectWriteFallback(false) comment in particular explains why rather than what, which is exactly the house style.

_{🤖 aethersdr-agent · cost: $8.8480 · model: claude-opus-5}

Issue fit

Yes. #5664 asks for five things, and each maps to a hunk:

• atomic whole-document replacement (QSaveFile with setDirectWriteFallback(false));
• surfaced failures (reportPersistenceFailure);
• acknowledgement only after commit (read, kill and compose build a candidate, save it, then adopt it);
• a message ID consumed only on commit;
• the previous document preserved.

I confirmed the transactional part with mutants (below). One regression came with it: the new failure reporting can now fire from a destructor, and in the app's own ownership shape that aborts.

Scope

Everything in the diff is explained by the issue: no CHANGELOG.md, no settings key, no public surface. Sockets: none; the test uses the existing in-memory frame injection. pms_mailbox_test matches no ci.yml -R filter, so the green checks compiled it and did not run it. It first runs in full-suite.yml.

Blockers

1. ~PmsMailbox can now emit activity into its already-destroyed parent dialog, and Qt aborts. (inline PmsMailbox.cpp:100)

Before this PR, saveHeard() was silent on failure. Now the destructor's save reaches emit activity(...). In the app, m_pms is a child of Ax25HfPacketDecodeDialog (Ax25HfPacketDecodeDialog.cpp:852), wired directly to the derived member appendSystemLine (:1253). The dialog's destructor never disconnects it, and QWidget::~QWidget deletes children before ~QObject disconnects anything. So a heard-list write that fails at shutdown calls a member function on a dialog whose derived part is gone — the storage-failure path this PR exists to make safe.

Reproduced with a review-only probe using the same shape: a QWidget subclass, a PmsMailbox child, activity connected to a derived member slot, and heard.json blocked by a directory:

~Dlg body done
aether.ax25: PMS could not save heard stations: Filename refers to a directory
ASSERT failure in QWidget: "Called object is not of the correct type (class destructor may have already run)", file .../QtCore.framework/Headers/qobjectdefs_impl.h, line 107
exit=134

That is with assertions enabled in the local Qt build (Homebrew, macOS). Without them, the same call runs a member function on a destroyed object. The one-line suggestion inline blocks signals for the destructor's save only, and keeps the qCWarning. With it applied, the probe reports calls into destroyed derived dialog: 0 and exits 0, and pms_mailbox_test still passes.

Nits (non-blocking)

• The test never reaches the direct-write fallback it disables (inline PmsMailbox.cpp:40). With setDirectWriteFallback(true), the whole suite still passes: a directory at the target path fails QSaveFile::open before any fallback applies. A read-only store directory is the fixture that exercises it (inline at pms_mailbox_test.cpp:666).
• The recovered compose is not checked for the retained body (inline pms_mailbox_test.cpp:625). Clearing m_draftLines on the failure path still passes, because an empty-body message also reads SAVED with count 2.
• I agree with the earlier review's compose-trap note (no abort verb while storage stays broken). It's not repeated inline.

What I tried to break

• Built pms_mailbox_test at 302c6688 (Debug, macOS, -j4): all pass.

• Mutants. Caught:

  • skipping commit(): 23 failures, including all three hard-link snapshot checks, so atomic replacement is really exercised;
  • adopting the kill before its save;
  • adopting the read flag on failure;
  • consuming an ID on a failed compose;
  • not emitting activity;
  • the heard save ignoring failure.

  Survived:

  • the fallback flag (nit 1);
  • dropping the draft on failure (nit 2);
  • write() < 0 in place of the full-length check, which can't be triggered without an injection seam, so not a finding.

  A plain-QFile writer mutant didn't compile (no commit()) and was not run.

• Suggested tests, checked both ways:

  • the read-only-directory case passes at the head, and fails 3 checks under the fallback mutant;
  • the body check (body == "body", since the first compose line is the subject) passes at the head and fails under the draft-dropping mutant.

• Heard-save cadence. recordHeard saves only for a new station, so a persistently failing store cannot flood the activity log at packet rate.

• Interaction with #5659, which also edits PmsMailbox and this test target: git merge-tree shows no conflict between the two heads.

• Not driven in the app: the bridge has no PMS frame or storage-fault injection, and teardown ordering is what the probe covers.

Recommendation

Request changes — one line. The persistence fix is right and well tested. Blocker 1 is a crash the PR introduces on the failure path it is fixing, and the validated suggestion closes it. The two test additions are optional, but each closes a mutant that currently survives.

Issue fit

Yes. #5664 requires atomic whole-document replacement, surfaced open/write/commit failures, acknowledgement only after commit, nextId consumed only on a committed write, and preservation of the previous document. The hunks map 1:1: writeJsonAtomically (QSaveFile + setDirectWriteFallback(false)), reportPersistenceFailure, candidate-vector then adopt in cmdRead / cmdKill / finishCompose, and hard-link assertions in testPersistenceIsAtomicAndTransactional.

m_heard / m_callers still mutate before save and keep the mutation on failure. The issue scopes transactional consistency to message data / read flags / next ID, so that is not a miss.

Scope

No CHANGELOG, settings, public/protocol surface, or radio-family change. Sockets: none in the new test (in-memory onAirFrame / Frame::encode() only).

Blockers

1. ~PmsMailbox can emit activity into an already-destroyed parent dialog. (inline PmsMailbox.cpp:103)

This is the same defect K5PTB requested changes for; it is still present on 302c6688. Before this PR, saveHeard() was silent on failure. Now the destructor reaches reportPersistenceFailure → emit activity(...). In the app, m_pms is a child of Ax25HfPacketDecodeDialog (Ax25HfPacketDecodeDialog.cpp:852) and activity is connected to the derived slot appendSystemLine (:1253). QWidget::~QWidget deletes children after the derived destructor has finished, so a failing heard save at teardown calls a member on a dialog whose derived part is gone. Qt debug builds abort; release builds are UAF.

Fix: block signals around the destructor save (keep qCWarning). Do not merge until that lands.

Nits (non-blocking)

• Compose has no abort while storage stays down (finishCompose(false) is still unused). Better than the old false SAVED, not a merge gate.
• m_draft = candidate after a successful compose is unused until the next cmdSendBegin resets it.
• The directory-at-path fixture does not exercise setDirectWriteFallback(false); that is still the correct flag.
• pms_mailbox_test is not in ci.yml -R filters; default CI compiled it, did not run it.

Verification

Source review of the three-file diff at 302c66883d64cd97b945ac078539bc911973a065 (blob SHAs match the PR snapshot). No local build this pass: the destructor emit is a demonstrated defect from the app ownership graph plus K5PTB's probe, and it already blocks merge.

CI on this SHA: CI build / check-macos / check-windows SUCCESS; Static checks SUCCESS. Required checks green does not override the lifetime defect.

Recommendation

Request changes. Persistence semantics match #5664. Do not merge until the destructor does not emit into a destroyed parent. After that one-line (or equivalent) fix, re-run pms_mailbox_test and this review can approve.

Issue fit

Yes. #5664 still maps 1:1. Follow-up daf401dc blocks activity in ~PmsMailbox via QSignalBlocker so a failed heard save cannot reach Ax25HfPacketDecodeDialog::appendSystemLine after the derived destructor.

Scope

Unchanged plus the destructor guard and a regression in pms_mailbox_test. No public surface.

Blockers

None on this SHA.

Verification

• Built [pms_mailbox_test](/tmp/aether-review-pr-5666/build/pms_mailbox_test) (app not rebuilt).
• QT_QPA_PLATFORM=offscreen ctest -R '^pms_mailbox_test$' passed (1/1).
• Mutation: drop QSignalBlocker → FAIL destructor save does not emit activity on failure; restored and passed.

Recommendation

Approve. Merge when required CI on daf401dc is green.